[Variant] Add VariantBuilder values check - #10016
Conversation
|
@alamb could you run some benchmarks?
|
|
run benchmarks variant_builder variant_kernels variant_validation |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing variantbuilder-count-check (f372e3a) to 1377761 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing variantbuilder-count-check (f372e3a) to 1377761 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing variantbuilder-count-check (f372e3a) to 1377761 (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
|
everything looks fine other than - |
klion26
left a comment
There was a problem hiding this comment.
LGTM, thanks for the contribution
|
|
|
@scovich PTAL |
scovich
left a comment
There was a problem hiding this comment.
I took a quick skim but I don't feel like I fully understand what the PR is doing nor the ramifications. Work+travel is crazy this week and I'll be off-grid next week, so I'll either need to step aside or come back to this in July with a clearer mind and more time on my hands.
Really fast take is:
- Ideally the variant builder API should make it structurally impossible to create more than one top-level variant value. This PR doesn't do that. I don't know whether it's even possible to do that, tho, or how hard/invasive it would be
- I'm not sure this PR blocks finishing an empty variant builder (maybe it does and I just didn't see it in my super-shallow skim)
- There seems to be a potential performance impact (all the careful inlining/cold annotations, flagged benchmark result, etc)
- It's not immediately obvious that the problem being fixed is causing a lot of pain
It does that. It wasn't obvious because I forgot to add unit tests that shows the behavior - added here 6372487
It does that as well. Check the unit tests here 6372487
That's true, but I inlined/cold'ed as I could and the behavior stays very to original on my machine ( - "it works on my computer" 🤓 )
Some unit tests had errors I fixed here. Example - https://github.com/apache/arrow-rs/pull/10016/changes#diff-3474fdf73b884864c8f3febda29b921f2604e48b61e46e9d78435af4a2972949L469-L473 |
Please take your time |
|
Thanks for the extra info, that definitely reduces my worry. I haven't properly reviewed the code so I won't stamp, but no objections to merging based on the stamps already present. |
Which issue does this PR close?
Rationale for this change
VariantBuilderaccepts 0/1+ values on finish.What changes are included in this PR?
VariantBuildertry_new_list/try_new_objecttoVariantBuilderVariantBuilderwithWritableMetadataBuilderin tests that only build metadata.Are these changes tested?
[#inline]Are there any user-facing changes?